Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added requestConnectionPriority methods. #112

Closed
wants to merge 1 commit into from

Conversation

svangsgaard
Copy link

I've tried to implement requestConnectionPriority support.
Thank you for an awesome library.

@dariuszseweryn
Copy link
Owner

Hello @svangsgaard

Thank you for your pull request - this is a great idea! Unfortunately I cannot merge it yet, there are few things that need to be clarified:

  1. Setting the connection priority may fail and your code does not inform the user about the result
  2. Have you tested wether requesting connection priority may be called at any time or does it have to be called when no bluetooth operation is executed at that time? If it is the case we would need to make the request asynchronous and create a RxBleRadioOperationRequestConnectionPriority and related tests
  3. It may be also worth to discuss if a High Priority or Low Priority connection would have a lifecycle similar to normal connection and / or notifications. I.E. when user will request high priority connection it will be returned in Observable<RxBleConnectionHighPriority> which would have almost the same interface as RxBleConnection but would not be able to change the priority. When the user would unsubscribe from the Observable<RxBleConnectionHighPriority> the priority would be brought back to balanced state. What is your use-case?
  4. Your commit introduces formatting changes in JavaDoc which are not related to the topic and should be reverted.

After all I am looking forward to have this feature after we will discuss and implement it in a proper way.

Best Regards

@svangsgaard
Copy link
Author

Hi @dariuszseweryn

You're right. I need to change those things in the code.
I have thought about how to implement it, maybe a better solution is to provide an extra parameter to setupNotification aka setupNotification(MY_UUID, CONNECTION_PRIORITY_HIGH), then the priority is automatically reverted when the notification is released.

That will solve my current problem. What do you think?

Regards.

@dariuszseweryn
Copy link
Owner

I don't think that binding connection priority to setting up notifications is a good idea. I can imagine at least a situation where user would like to transmit data to the peripheral at increased rate.

Could you describe your use case in greater detail? Does your peripheral transmit a lot of data via notifications to the phone?

I am on the fence if introducing state to the connection via setting the connection priority is a good idea. Maybe it would be better to emit RxBleConnectionHighPriority but what if at the same time the user would also request low priority? Then an error should be emitted.

Either way - if you could confirm the point number 2 from my previous post - it would be clear if we need a separate operation.

@dariuszseweryn
Copy link
Owner

@svangsgaard Any hints you can provide?

gabrieloshiro added a commit to samsao/RxAndroidBle that referenced this pull request Feb 1, 2017
It allows users to request a certain priority for each connection.
@mikolak
Copy link
Collaborator

mikolak commented May 29, 2017

Now implemented. #111

@mikolak mikolak closed this May 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants